Conversation
Bewinxed
force-pushed
the
bewinxed/add-scim-v2
branch
2 times, most recently
from
f514de5 to
7a60f1b
Compare
Bewinxed
force-pushed
the
bewinxed/add-scim-v2
branch
from
72e8e4d to
8450879
Compare
Pull Request Test Coverage Report for Build 21770818575Details
💛 - Coveralls |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Bewinxed
force-pushed
the
bewinxed/add-scim-v2
branch
from
7fe64d5 to
9b1d88b
Compare
- Add FindUsersByProviderWithFilter for SCIM user listing - Add FindSCIMGroupsBySSOProviderWithFilter for group listing - Make external_id nullable, add case-insensitive displayName index - Validate user belongs to SSO provider before adding to group
…cap filter length, exclude members from group list by default
…eneric group error
cstockton
left a comment
cstockton
left a comment
There was a problem hiding this comment.
I haven't given this a full review yet, but one change we will need is to merge the migrations into one. This will lower the possibility of migration errors.
| return nil | ||
| } | ||
|
|
||
| if err := user.Ban(tx, time.Duration(math.MaxInt64), &scimDeprovisionedReason); err != nil { |
There was a problem hiding this comment.
🟠 Severity: HIGH
Privilege escalation: scimDeleteUser unconditionally overwrites existing ban reasons with SCIM_DEPROVISIONED. A SCIM admin can bypass a Super Admin's platform-wide ban by deleting then re-creating the user, which triggers an unban because the reason now matches the SCIM deprovisioning tag.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Prevent SCIM from overwriting non-SCIM ban reasons to avoid privilege escalation. Before calling user.Ban() with scimDeprovisionedReason, check if the user is already banned with a different reason. If the user has been banned by a Super Admin for security reasons (e.g., 'Compromised Account'), SCIM should not be allowed to overwrite that ban reason. This prevents a SCIM admin from bypassing the platform-wide ban by deprovisioning (which would overwrite the reason) and then re-provisioning (which triggers an unban because the reason now matches scimDeprovisionedReason). The same vulnerability exists in scimPatchUser and scimReplaceUser functions and should be addressed there as well.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if err := user.Ban(tx, time.Duration(math.MaxInt64), &scimDeprovisionedReason); err != nil { | |
| // Prevent overwriting non-SCIM ban reasons to avoid privilege escalation | |
| if user.IsBanned() && (user.BannedReason == nil || *user.BannedReason != scimDeprovisionedReason) { | |
| return apierrors.NewSCIMBadRequestError("User is banned by administrator and cannot be deprovisioned via SCIM", "mutability") | |
| } | |
| if err := user.Ban(tx, time.Duration(math.MaxInt64), &scimDeprovisionedReason); err != nil { | |
| return apierrors.NewSCIMInternalServerError("Error deprovisioning user").WithInternalError(err) | |
| } |
internal/api/scim.go
Outdated
| if user.Identities[i].Provider == providerType { | ||
| if user.Identities[i].IdentityData != nil { | ||
| delete(user.Identities[i].IdentityData, "external_id") | ||
| } | ||
| if err := tx.UpdateOnly(&user.Identities[i], "identity_data"); err != nil { |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
Stale authentication mapping: Removing externalid via PATCH only deletes it from IdentityData but leaves the provider_id column intact in the identities table. The user remains able to authenticate using the identifier the administrator intended to revoke.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: When removing the externalid via SCIM PATCH, both the identity_data JSON field and the provider_id column must be cleared. Currently only identity_data is updated, leaving the provider_id intact, which allows the user to continue authenticating with the old identifier. Clear the provider_id field by setting it to an empty string and include both "provider_id" and "identity_data" in the UpdateOnly call, mirroring the pattern used in the 'add' operation at lines 565-571.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if user.Identities[i].Provider == providerType { | |
| if user.Identities[i].IdentityData != nil { | |
| delete(user.Identities[i].IdentityData, "external_id") | |
| } | |
| if err := tx.UpdateOnly(&user.Identities[i], "identity_data"); err != nil { | |
| if user.Identities[i].Provider == providerType { | |
| user.Identities[i].ProviderID = "" | |
| if user.Identities[i].IdentityData != nil { | |
| delete(user.Identities[i].IdentityData, "external_id") | |
| } | |
| if err := tx.UpdateOnly(&user.Identities[i], "provider_id", "identity_data"); err != nil { | |
| return apierrors.NewSCIMInternalServerError("Error updating identity").WithInternalError(err) | |
| } | |
| break | |
| } |
3 participants
What kind of change does this PR introduce?
Feature - adds SCIM v2 provisioning support for enterprise SSO providers
This is a complete, general implementation inspired by the needs of this PR #2115
What is the current behavior?
Currently there's no way for identity providers (Okta, Azure AD, OneLogin, etc.) to automatically provision and deprovision users. Admins have to manually manage user accounts when employees join or leave, which is error-prone and doesn't scale for enterprise customers.
What is the new behavior?
Adds full SCIM v2 (RFC 7644) support, allowing identity providers to:
Endpoints added:
GET/POST /scim/v2/Users- list and create usersGET/PUT/PATCH/DELETE /scim/v2/Users/{id}- manage individual usersGET/POST /scim/v2/Groups- list and create groupsGET/PUT/PATCH/DELETE /scim/v2/Groups/{id}- manage individual groups/scim/v2/ServiceProviderConfig,/scim/v2/Schemas,/scim/v2/ResourceTypesAuthentication: Bearer token per SSO provider (stored as SHA-256 hash). Tokens are 160 bits generated from
crypto/randwith ascim_prefix.Filtering: Full RFC 7644 filter support using the
scim2/filter-parserlibrary - supportseq,ne,co,sw,ew,pr,gt,ge,lt,leoperators withand/or/notlogic.IdP compatibility: Tested with Azure AD quirks (booleans as strings, case-insensitive displayName uniqueness).
SCIM routes skip
isValidExternalHostmiddlewareSCIM routes intentionally do not go through the
isValidExternalHostmiddleware. This is by design because:isValidExternalHost(validating redirect destinations)Additional context
I tried my best to make the implementation fit within the current tenant/user model instead of new tables for everything, adding schema changes only when necessary.
Some compliance work might be needed for other nuances with other SCIM providers (I've tested Microsoft Azure).
Deviations from RFC 7643
Some deviations from the RFC for SCIM v2 were done that relates to Supabase Auth:
New dependencies
Schema Changes
Testing
All tests were based on the assumptions that the Azure Validation Tool expects, currently all passing.